Major API refactor and additional features/fixes #9

andywaplinger · 2018-01-23T18:18:30Z

Fixes #7, fixes #3.

Major Changes

Updated API to MBTA API V3; V2 will be deprecated in April 2018
Split API calls across multiple functions with deferred callbacks to get all required data
- Requires the use of jQuery 3 for deferred callbacks
Modified names of some subway stops to better reflect what they're typically called; users using affected stops will need to update the name in their config
Added commuter rail stops and bus stops
Added ability to switch been predictions and schedules using predictedTimes boolean
Added ability to display departure time using showDepartTime boolean
- Corrected the use of departure time vs arrival time
Added colorIcons boolean so vehicle icons reflect their line color

Minor Changes

Trips are sorted by ETA, not departure time
Trips filtered by ETA when maxTime is set
Added bus route number to trip description
Fixed alerts and enabled marquee scrolling
Added dictionary for trip directions (more info below)
Added "mins" or "min" to ETA time so it's more human-readable
Accounted for trips with no specified arrival or departure times
Accounted for no alerts at a stop if alerts are turned on

Known Issues

When there are multiple alerts on a trip, only the first alert is shown
Direction filtering is not yet functional, but some groundwork has been laid

edward-shen · 2018-01-23T19:10:17Z

Thanks so much for the PR!

I'll do a proper review soon, but for now some quick notes:

Resolves #7. Does this resolve #4, #3, and #1? Did I forget anything?

I took a quick review at the changes with the API V3: looks like we can remove the updateInterval limit since each key can handle 1k request/minute. This is more of a note for myself, but you're free to make a change that limits it to 1 (split-second updating is a little unnecessary in all use cases).

I'm still quite new to Boston; what are some colloquial examples of subway stops? And can you tell me which ones were changed? (For documentation purposes)

Can you also update the readme for getting an API v3 key? I think we just need to change the old link to https://api-v3.mbta.com/register

andywaplinger · 2018-01-23T20:20:19Z

You're welcome!

I'd say it definitely solves #3 and #7, and solves or, at least, helps #4. I'm doing more testing now and I wouldn't say that #1 is fixed. The alerts work better, but still only work well in full-width positions. I'll have to dig further into that.

I just tested setting the limit to 1s and quickly received 429 Too Many Requests. I'd recommend keeping updateInterval, but I can change it to 10s default.

Most of the names I changed had "Station" at the end when that's typically dropped, i.e. "Riverside Station" is now "Riverside", or "Boylston Street Station" is just "Boylston". Here's a sheet with the updated stations - updated_stops.xlsx

I'll also update the URL in the readme.

edward-shen · 2018-01-24T03:02:33Z

It's default limited to 10s, so you shouldn't need to worry about that.

edward-shen · 2018-01-24T18:35:19Z

Made some minor changes, you might need to merge changes from master to resolve conflicts.

andywaplinger · 2018-01-25T20:13:36Z

Cool, I merged your changes. Should be good to go now.

Edit: made a commit error originally, but it should be fixed.

edward-shen

If you could fix and/or explain some of the notes I've made, I would greatly appreciate it. Sorry it took so long, there was a lot more code than I expected and I got caught up with work.

edward-shen · 2018-01-25T19:02:25Z

MMM-MBTA.js

+ }
+ if (this.config.direction.includes("Eastbound")) {
+ this.filterDirection.push("1");
+ }


Wouldn't a switch case be better here? We can then use fall-through to make this a lot cleaner.

Sure, just like this?

switch (this.config.direction) { case "Southbound": case "Westbound": case "Outbound": this.filterDirection.push("0"); break; case "Northbound": case "Eastbound": case "Inbound": this.filterDirection.push("1"); break; }

edward-shen · 2018-01-25T19:04:46Z

MMM-MBTA.js

+ break;
+ default:
+ descCell.innerHTML += " | " + routeId;
+ }


What are SL1-4 and CT1-3? I'm assuming CT# stands for Commuter Train, but what are SL1?

Can you add some comments?

Those are their public-facing route names. Aside from those, every other bus uses its routeId as it's public route name.

edward-shen · 2018-01-25T19:10:21Z

MMM-MBTA.js

+ arrTimeCell.innerHTML = "No arrival";
+ } else {
+ arrTimeCell.innerHTML = moment.unix(this.stationData[i].preArr).format("h:mm");
+ }


I think doing this is cleaner:

if (isNaN(this.stationData[i].preArr) === true) { arrTimeCell.innertHTML = "No arrival"; } else { if (config.timerFormat === 24) { arrTimeCell.innerHTML = moment.unix(this.stationData[i].preArr).format("H:mm"); } else { arrTimeCell.innerHTML = moment.unix(this.stationData[i].preArr).format("h:mm"); } }

What does no arrival mean? When does a mode of transportation show this?

Good point. Changed.

Sometimes arrival_time is null. If it's a bus/subway, I'm not sure why. But if it's a train, it's typically because it already arrived and is waiting to depart.

Oh god, we need to check for null?

If that's the case, we can check nullity just by doing the following:

if (!this.stationData[i].preArr) { ... }

This is because null is a falsey value, so when you negative null, it returns true.

Thanks javascript.

Ahh, right, one of my favorite things about JS...

edward-shen · 2018-01-25T19:11:35Z

MMM-MBTA.js

+ depTimeCell.innerHTML = moment.unix(this.stationData[i].preDt).format("h:mm");
+ }
+ }
+ row.appendChild(depTimeCell);


Same as above.

Sometimes departure_time also comes in null. No idea why.

edward-shen · 2018-01-25T19:13:43Z

MMM-MBTA.js

+
+ function onlyUnique(value, index, self) { 
+ return self.indexOf(value) === index;
+ }


Unused function, please remove.

Slipped through

edward-shen · 2018-01-25T19:24:58Z

MMM-MBTA.js

+ if (!this.config.predictedTimes) {
+ url += "&page[limit]=25";
+ url += "&filter[min_time]=" + moment().format("HH:mm");
+ url += "&filter[max_time]=" + moment().add(3, 'h').format("HH:mm");


Can you please leave a comment explaining this is necessary, for documentation's sake?

Added

Page and time limits necessary because otherwise "schedules" endpoint gets every single schedule

Sorry, I meant to clarify moment.add(3, 'h'), but that's also good to have too

Oh, gotcha. Does that comment cover enough ground for you? Or do you want me to another specifically about limiting the time to 3 hours from the current time?

edward-shen · 2018-01-25T19:30:13Z

MMM-MBTA.js

+ alertUrl += "?api_key=" + this.config.apikey;
+
+ return alertUrl;
+ },


It might be a good idea to make some sort of enumeration type and merge tripURL and routeURL with this.

JS enumerations don't really exist, but there are ways to emulate them.

Regardless, I feel like making a switch statement would make reduce the amount of duplicated code.

Fixed using a single function.

edward-shen · 2018-01-25T19:35:05Z

MMM-MBTA.js

+ preDt: "None",
+ preArr: "None",
+ preETA: "None"
+ });


I'm a little uncomfortable requiring our code to have a "default" state, because that implies that our code implicitly requires our stationData requires a minimum length of one--it's a code smell, and a very smelly one at that.

If there isn't one station, then getDom() skips the for loop that renders all trip data. I can include an if statement before the loop that catches when stationData.length === 0 and renders just the symbolCell and descCell, unless you have a different idea.

Fixed - opted for the if statement.

edward-shen · 2018-01-25T19:39:38Z

MMM-MBTA.js

 }
+ preDt = parseInt(moment(data.data[pred].attributes.departure_time).format("X"));
+ preArr = parseInt(moment(data.data[pred].attributes.arrival_time).format("X"));
+ preETA = moment(data.data[pred].attributes.arrival_time).diff(moment(), 'seconds') - 30; //Better safe than sorry?


Do we want to be truthful and inaccurate, or lie and "in good faith"?

Haha, why not both? There's already the disclaimer, which is good. We could also include that we've added a 30s buffer to help ensure they're on time.

edward-shen · 2018-01-26T00:48:30Z

MMM-MBTA.js

+ preDt: "None",
+ preArr: "None",
+ preETA: "None"
+ });


I would rather check if the stationData is empty when we're rendering, rather than making an "fake" station.

It's a misrepresentation of our data definition.

edward-shen · 2018-01-27T01:25:56Z

Can you make sure you're pushing to your changes to github? I can't see local git changes!

andywaplinger · 2018-01-27T04:22:21Z

Oh yeah, sorry, I've been making a few of these changes before pushing. I'll push what I have now.

- Change `direction` filtering to switch case - Clean up description, arrival, departure, and alerts - Allow users to retrieve as many schedules as they want

- Retrieve all alerts from all predictions - Changed icon when no upcoming trips; icon is red when `colorIcons` is `true`

edward-shen

Just some last few style remarks!

One last thing: Can you provide me with a list of stations that show all your new features? It's for testing purposes, so I can triple check your code.

After I do that, then it should be on the home stretch!

edward-shen · 2018-01-28T08:02:31Z

MMM-MBTA.js


- return alertUrl;
+ return url


Missing semi-colon

edward-shen · 2018-01-28T20:12:34Z

MMM-MBTA.js

- preAwayCell.innerHTML = minutes + ":" + seconds;
- } else {
- preAwayCell.innerHTML = seconds;
+ if (isNaN(this.stationData[i].routeId) === false) {


Same thing here, can just be

if (this.stationData[i].routeId) { ... }

What do you mean? Only bus routes use straight numbers. Everything else is alphanumeric.

I can replace it with if ($.isNumeric(this.stationData[i].routeId)) instead so it's a little cleaner and clearer. Checking whether something is not not a number is kinda dumb.

edward-shen · 2018-01-28T20:14:01Z

MMM-MBTA.js

+ var seconds = preETATime % 60;
+
+ if (this.config.showMinutesOnly) {
+ if (isNaN(minutes) === true) {


This also a null check?

Yeah. Just replaced it with if (!minutes).

edward-shen · 2018-01-28T20:16:23Z

MMM-MBTA.js

+ routeId = data.data[pred].relationships.route.data.id;
+ tripId = data.data[pred].relationships.trip.data.id;
+ alertIds = data.data[pred].relationships.alerts.data;
+debugger


extraneous debugger

edward-shen · 2018-01-28T20:16:44Z

MMM-MBTA.js

+ alert = alertsParse[i].data.attributes.header;
+ alertsArray.push(alert);
+ }
+ };


extra semi colon

Actually if you want this to look cleaner you can do a for...of loop:

for (let alert of alertsParse) { if (alert.data) { alertsArray.push(alert.data.attributes.header); } }

Optional ofc, but nice learning point and could help make code cleaner in the future.

I'm all about cleaner code. I'll change it.

edward-shen · 2018-01-28T20:29:08Z

Would like to thank you again for such a large contribution to the repo; in fact, it looks like after this merge, you'll have more lines of code than me! 😆

LMK when you're done with the last few changes, otherwise LGTM.

andywaplinger · 2018-01-28T21:43:47Z

You're very welcome! It was a really useful learning experience for me, and I think it's a great little app. I'm happy to help make it even better. 😄

Let's merge this! 👍

andywaplinger · 2018-01-29T02:37:33Z

For station testing, I've been using North Station (multiple subway lines and typically has one or two alerts), Central Square (typically has no alerts), South Station (buses, subways, and trains, sometimes with alerts), and random bus and commuter rail stops. Every feature should work, regardless of the stop. 😄

edward-shen · 2018-01-29T05:36:41Z

Congratulations on your first PR! 🎉

Major API refactor and additional features/fixes

907079c

Merged with master

421c812

andywaplinger added 2 commits January 25, 2018 15:17

Merged with master

4b4d4c3

Merged with master

5f2a8e7

edward-shen reviewed Jan 26, 2018

View reviewed changes

andywaplinger added 3 commits January 26, 2018 23:28

- Change updateInterval default to 10s

9c87c1c

- Change `direction` filtering to switch case - Clean up description, arrival, departure, and alerts - Allow users to retrieve as many schedules as they want

Merge route, trip, and alert URL functions

502865c

- Removed needing default state

4c2a9e6

- Retrieve all alerts from all predictions - Changed icon when no upcoming trips; icon is red when `colorIcons` is `true`

edward-shen reviewed Jan 28, 2018

View reviewed changes

andywaplinger added 2 commits January 28, 2018 16:01

Add functionality to auto-hide Alerts section when empty

93e878e

Minor code cleanup

89a3060

Replaced double negative NaNs

4718cac

edward-shen approved these changes Jan 29, 2018

View reviewed changes

edward-shen merged commit 7cf8b99 into edward-shen:master Jan 29, 2018

Major API refactor and additional features/fixes #9

Major API refactor and additional features/fixes #9

Conversation

andywaplinger commented Jan 23, 2018 • edited by edward-shen Loading

edward-shen commented Jan 23, 2018

andywaplinger commented Jan 23, 2018

edward-shen commented Jan 24, 2018

edward-shen commented Jan 24, 2018

andywaplinger commented Jan 25, 2018 • edited Loading

edward-shen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andywaplinger Jan 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andywaplinger Jan 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edward-shen commented Jan 27, 2018

andywaplinger commented Jan 27, 2018

edward-shen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edward-shen Jan 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edward-shen commented Jan 28, 2018

andywaplinger commented Jan 28, 2018

andywaplinger commented Jan 29, 2018

edward-shen commented Jan 29, 2018 • edited Loading

andywaplinger commented Jan 23, 2018 •

edited by edward-shen

Loading

andywaplinger commented Jan 25, 2018 •

edited

Loading

andywaplinger Jan 26, 2018 •

edited

Loading

andywaplinger Jan 26, 2018 •

edited

Loading

edward-shen Jan 28, 2018 •

edited

Loading

edward-shen commented Jan 29, 2018 •

edited

Loading